Clarify layer ordering #349

vbatts · 2016-09-29T12:43:31Z

This series of commits touches on the places that ordering is applied/unpacked.
Attempting to have consistent wording.

Fixes #276

vbatts · 2016-09-29T12:44:13Z

@opencontainers/image-spec-maintainers PTAL (and @AaronLehman too, please :-)

wking · 2016-09-29T17:22:06Z

config.md

    - **diff_ids** *array*, REQUIRED

-       An array of layer content hashes (`DiffIDs`), in order from bottom-most to top-most.
+       An array of layer content hashes (`DiffIDs`), in stack order from bottom-most to top-most (i.e. from diff_ids[0] to diff_ids[len(diff_ids)]).


“diff_ids[0] to diff_ids[len(diff_ids)]” → “diff_ids[0] to diff_ids[len(diff_ids)]”.

wking · 2016-09-29T17:26:14Z

manifest.md

 The second goal is to allow multi-architecture images, through a "fat manifest" which references image manifests for platform-specific versions of an image.
 The third goal is to be translatable to the [OpenContainers/runtime-spec](https://github.com/opencontainers/runtime-spec)

+This section defines the `application/vnd.oci.image.manifest.list.v1+json` and `application/vnd.oci.image.manifest.v1+json` [media type](media-types.md).


Can we break this up into two files? Manifest lists and manifests seem as separate as manifests and configs. And while I like the commits that clearly list the media types being defined, I think that change is orthogonal to the order-clarification. Maybe it belongs in a separate PR?

This is OK, but separate PR.

wking · 2016-09-29T17:29:57Z

manifest.md

-    Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.
-    The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.
+    Subsequent layers MUST then follow in stack order.
+    The final filesystem layout MUST be from [applying](layer.md#applying) the layers in stack order (i.e. from layers[0] to layers[len(layers)]).


I still think we need to specify something about the initial state before the layers are applied (#318). I also changed “be” to “match” in that PR to allow for implementations like union mounting.

Also backticks around the layers[…] stuff.

Agreed, but lets leave this for #318

wking · 2016-09-29T17:35:32Z

image-layout.md

+Tooling can:
+* convert a given ref into a runnable OCI Image Format by finding an appropriate manifest from the manifest list
+* apply the [filesystem layers](layer.md) per the [manifest layers ordering](manifest.md#image-manifest-property-descriptions)
+* convert the image configuration into an [OCI Runtime config.json](https://github.com/opencontainers/runtime-spec)


All of these steps are required to create a runable bundle. How about:

Given an image layout and a ref, a tool can create an OCI Runtime Specification bundle by:

Following the ref to find a manifest, possibly via a manifest list,

Unpacking the filesystem layers in the correct order, and

Converting the image configuration into an OCI Runtime Specification config.json.

[edit: “… a runnable OCI Runtime…” → “… an OCI Runtime…”.]

philips

LGTM overall

philips · 2016-09-29T20:53:41Z

manifest.md

 The second goal is to allow multi-architecture images, through a "fat manifest" which references image manifests for platform-specific versions of an image.
 The third goal is to be translatable to the [OpenContainers/runtime-spec](https://github.com/opencontainers/runtime-spec)

+This section defines the `application/vnd.oci.image.manifest.list.v1+json` and `application/vnd.oci.image.manifest.v1+json` [media type](media-types.md).


This is OK, but separate PR.

philips · 2016-09-29T21:07:11Z

manifest.md

-    Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.
-    The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.
+    Subsequent layers MUST then follow in stack order.
+    The final filesystem layout MUST be from [applying](layer.md#applying) the layers in stack order (i.e. from layers[0] to layers[len(layers)]).


Agreed, but lets leave this for #318

vbatts · 2016-09-29T21:31:14Z

I see that #318 removes that line, but not how it clarifies anything. What
do you want me to change?

On Thu, Sep 29, 2016, 23:08 Brandon Philips [email protected]
wrote:

@philips commented on this pull request.

LGTM overall

In manifest.md
#349 (review)
:

@@ -5,6 +5,7 @@ The first goal is content-addressable images, by supporting an image model where
The second goal is to allow multi-architecture images, through a "fat manifest" which references image manifests for platform-specific versions of an image.
The third goal is to be translatable to the OpenContainers/runtime-spec

+This section defines the application/vnd.oci.image.manifest.list.v1+json and application/vnd.oci.image.manifest.v1+json media type.

This is OK, but separate PR.

In manifest.md
#349 (review)
:

@@ -159,8 +160,8 @@ Unlike the Manifest List, which contains information about a s
 Each item in the array MUST be a [descriptor](descriptor.md).
 The array MUST have the base image at index 0.
Subsequent layers MUST then follow in the order in which they are to be layered on top of each other.

The algorithm to create the final unpacked filesystem layout MUST be to first unpack the layer at index 0, then index 1, and so on.

Subsequent layers MUST then follow in stack order.

The final filesystem layout MUST be from applying the layers in stack order (i.e. from layers[0] to layers[len(layers)]).

Agreed, but lets leave this for #318
#318

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#349 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6RGYCtUG_2l8fSBD2wD6SHUL0LJAks5qvCiGgaJpZM4KJ8k6
.

philips · 2016-09-29T21:33:42Z

@vbatts don't worry about #318. The language in #318 is "+ The root filesystem MUST match the result of applying the entries to an empty directory in the listed order."

I think this is a good suggestion for this PR: #349 (comment)

wking · 2016-09-29T21:43:06Z

On Thu, Sep 29, 2016 at 02:31:15PM -0700, Vincent Batts wrote:

I see that #318 removes that line, but not how it clarifies
anything. What do you want me to change?

#318 forbids seeding the unpack directory location with any content
1 and changes “be” → “match”, but I'm ok with @philips' suggestion
to keep those in #318 2. If we punt those back to #318, the only
change I'd like to see for that line is the backticks 3.

vbatts · 2016-09-30T08:34:26Z

updated

jonboulle · 2016-09-30T13:33:33Z

layer.md


 This document describes how to serialize a filesystem and filesystem changes like removed files into a blob called a layer.
-One or more layers are ordered on top of each other to create a complete filesystem.
+One or more layers changesets are applied on top of each other to create a complete filesystem.


remove "changesets"

jonboulle · 2016-09-30T13:33:51Z

config.md

    - **diff_ids** *array*, REQUIRED

-       An array of layer content hashes (`DiffIDs`), in order from bottom-most to top-most.
+       An array of layer content hashes (`DiffIDs`), in stack order from bottom-most to top-most (i.e. from `diff_ids[0]` to `diff_ids[len(diff_ids)]`).


I really don't like introducing the word "stack" because it addles my poor brain

k. I'll just switch from the whole bottom to top, since we're not pushing and popping this list anyways.

jonboulle · 2016-09-30T13:34:03Z

image-layout.md

+Given an image layout and a ref, a tool can create an [OCI Runtime Specification bundle](https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/bundle.md) by:
+
+* Following the ref to find a [manifest](manifest.md#image-manifest), possibly via a [manifest list](manifest.md#manifest-list)
+* Apply the filesystem layers in the specified order


jonboulle · 2016-09-30T13:35:18Z

config.md

 An *Image* is an ordered collection of root filesystem changes and the corresponding execution parameters for use within a container runtime.
 This specification outlines the JSON format describing images for use with a container runtime and execution tool and its relationship to filesystem changesets, described in [Layers](layer.md).

+This section defines the `application/vnd.oci.image.config.v1+json` [media type](media-types.md).


I appreciate these but would also say a different PR..

Because stacks Signed-off-by: Vincent Batts <[email protected]>

Signed-off-by: Vincent Batts <[email protected]>

where as the data structures pointing to these chagesets can control their own ordering. Signed-off-by: Vincent Batts <[email protected]>

This long sentence is more clear as a list Signed-off-by: Vincent Batts <[email protected]>

vbatts · 2016-09-30T13:46:04Z

updated PTAL

jonboulle · 2016-09-30T13:51:44Z

lgtm

wking · 2016-09-30T14:38:40Z

image-layout.md

+Given an image layout and a ref, a tool can create an [OCI Runtime Specification bundle](https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/bundle.md) by:
+
+* Following the ref to find a [manifest](manifest.md#image-manifest), possibly via a [manifest list](manifest.md#manifest-list)
+* Applying the filesystem layers in the specified order


Make “Applying the filesystem layers” a link to layers.go#applying?

wking · 2016-09-30T14:38:56Z

image-layout.md

+
+* Following the ref to find a [manifest](manifest.md#image-manifest), possibly via a [manifest list](manifest.md#manifest-list)
+* Applying the filesystem layers in the specified order
+* Converting the [image configuration](config.md) into an [OCI Runtime Specification config.json](https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc2/config.md)


Backticks around config.json?

philips · 2016-09-30T15:57:26Z

LGTM

…ectory Point out that the layer's initial empty directory has unspecified attributes. Layer authors interested in the ownership and other attributes of the unpacked root directory should explicitly set an entry for it. This commit originally focused on requiring the initial directory to be empty before the layers are unpacked into it. But most of that was picked up in fed7241 (manifest: clarify the order of layers, 2016-09-29, opencontainers#349). I'd recommended "match" instead of "be" to allow folks to apply via a union filesystem or whatever. As long as the finished product is right, compliance testers and users should be satisfied. Signed-off-by: W. Trevor King <[email protected]>

wking reviewed Sep 29, 2016

View reviewed changes

philips reviewed Sep 29, 2016

View reviewed changes

vbatts force-pushed the layer_ordering branch 2 times, most recently from 2aa1478 to d4220e4 Compare September 30, 2016 08:34

jonboulle suggested changes Sep 30, 2016

View reviewed changes

vbatts mentioned this pull request Sep 30, 2016

*: consistent media-type indications #352

Merged

vbatts added 4 commits September 30, 2016 09:42

config: clarify the "bottom-most" verbiage

4d3fa68

Because stacks Signed-off-by: Vincent Batts <[email protected]>

manifest: clarify the order of layers

fed7241

Signed-off-by: Vincent Batts <[email protected]>

layer: changesets are applied

49b32b2

where as the data structures pointing to these chagesets can control their own ordering. Signed-off-by: Vincent Batts <[email protected]>

image-layout: clarify applying and ordering

52f5d46

This long sentence is more clear as a list Signed-off-by: Vincent Batts <[email protected]>

vbatts force-pushed the layer_ordering branch from d4220e4 to 52f5d46 Compare September 30, 2016 13:45

jonboulle approved these changes Sep 30, 2016

View reviewed changes

wking reviewed Sep 30, 2016

View reviewed changes

philips merged commit 261ed42 into opencontainers:master Sep 30, 2016

vbatts deleted the layer_ordering branch September 30, 2016 16:43

This was referenced Oct 1, 2016

manifest: Explicitly base manifest.layers on an empty directory #318

Closed

Make the order more clear #330

Closed

wking mentioned this pull request Oct 25, 2016

An image may have empty layers array #313

Closed

Clarify layer ordering #349

Clarify layer ordering #349

Uh oh!

Conversation

vbatts commented Sep 29, 2016

Uh oh!

vbatts commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philips left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 29, 2016

LGTM overall

This is OK, but separate PR.

Uh oh!

philips commented Sep 29, 2016

Uh oh!

wking commented Sep 29, 2016

Uh oh!

vbatts commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 30, 2016

Uh oh!

jonboulle commented Sep 30, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philips commented Sep 30, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wking Sep 29, 2016 •

edited

Loading

jonboulle commented Sep 30, 2016 •

edited by caniszczyk

Loading

philips commented Sep 30, 2016 •

edited by caniszczyk

Loading